Skip to content

fix: resolve 7 v1.4 bugs + remove orphaned segments#16

Merged
vishnujayvel merged 4 commits into
mainfrom
fix/v1.4-bugfixes
Mar 5, 2026
Merged

fix: resolve 7 v1.4 bugs + remove orphaned segments#16
vishnujayvel merged 4 commits into
mainfrom
fix/v1.4-bugfixes

Conversation

@vishnujayvel

@vishnujayvel vishnujayvel commented Mar 5, 2026

Copy link
Copy Markdown
Owner

Summary

  • B1: Analytics DB wiring — config default analytics.enabled: true, stats.tsx error msg fix
  • B3: daemon_health segment renderer + heartbeat separation in daemon-process
  • B4: FeatureCard layout min-width fix for vertical text rendering
  • B5: Coaching tab Switch toggles + write_config() for YAML persistence
  • B6: Remove ai_ratio segment (function definition + BUILTIN_SEGMENTS registry entry)
  • B7: Status tab rewrite — 20-segment configurator with live preview, 3 tier groups
  • B8: TUI duplicate tab prevention via PID check in tui-launcher
  • Insights segments: confusing UX + timezone-blind computations #15: Insights UX — peak_hour timezone conversion (UTC→local), copy: "this session"/"30d"

Cleanup

  • Remove orphaned practice_breadcrumb segment (no feed producer since v1.1)
  • Remove dead timezone?: string from InsightsFeedConfig
  • BUILTIN_SEGMENTS now has exactly 20 entries, matching TUI ALL_SEGMENTS

Known follow-ups (not in this PR)

  • FeatureCard badge/border stays visually stale after Switch toggle
  • "30d" label hardcoded in insight segments (lies if stalenessDays ≠ 30)
  • yaml.dump strips YAML comments on config write

Test plan

  • npx vitest run tests/core/status-line.test.ts — 37 tests pass
  • npx vitest run tests/core/status-line/segments-new.test.ts — 25 tests pass (daemon_health: 4 new)
  • npx vitest run tests/core/status-line/two-tier.test.ts — 18 tests pass
  • npx vitest run tests/integration/pipeline-wiring.test.ts — 21 tests pass
  • Verified ai_ratio and practice_breadcrumb have zero references in src/
  • Verified Python TUI segments match TS BUILTIN_SEGMENTS (diff = empty)
  • Manual: cd tui && uv run python -m hookwise_tui → Status tab shows 20 toggles
  • Manual: Coaching tab Switch toggles persist to ~/.hookwise/hookwise.yaml

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Tech Stack documentation page added
    • Agents list and daemon-health shown in status display
    • Interactive coaching toggles in the TUI
    • Three additional built-in feeds (practice, weather, memories)
  • Improvements

    • Analytics enabled by default
    • Status UI reorganized into tiered segment groups with optional separator and middle segments
    • Better duplicate-TUI launch detection
    • Platform documentation expanded with core principles
    • Peak-hour calculations converted from UTC → local
  • Changes

    • AI-authorship metrics removed from analytics
    • Clarified analytics-related error messaging

Bug fixes:
- B1: Analytics DB wiring — config default, stats.tsx error msg
- B3: daemon_health segment renderer + heartbeat separation
- B4: FeatureCard layout min-width
- B5: Coaching tab Switch toggles + config persistence via write_config()
- B6: Remove ai_ratio segment (function + registry) from BUILTIN_SEGMENTS
- B7: Status tab rewrite — 20-segment configurator with live preview
- B8: TUI duplicate tab prevention in tui-launcher
- #15: Insights UX — timezone conversion for peak_hour, "this session"/"30d" copy

Cleanup:
- Remove practice_breadcrumb segment (no producer, orphaned since v1.1)
- Remove dead timezone field from InsightsFeedConfig
- BUILTIN_SEGMENTS: 20 entries, exact match with TUI ALL_SEGMENTS

Tests: 101/101 segment tests pass, 1315/1437 total (10 pre-existing SQLite failures)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Mar 5, 2026

Copy link
Copy Markdown

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: f2aef514-7812-4f0c-be5c-4deb9a76c828

📥 Commits

Reviewing files that changed from the base of the PR and between 768a5e0 and 6cf685d.

📒 Files selected for processing (2)
  • hooks/agent-tracker.sh
  • src/core/status-line/two-tier.ts

📝 Walkthrough

Walkthrough

Enabled analytics by default, split heartbeat responsibilities with distinct cache keys/TTLs, added daemon heartbeat refresh, expanded built-in feeds, reworked status-line to add agents/daemon_health and remove ai_ratio/practice_breadcrumb, added TUI PID/config persistence and interactive coaching toggles, and updated matching tests.

Changes

Cohort / File(s) Summary
Docs & Philosophy
docs/.vitepress/config.ts, docs/philosophy.md
Added "Tech Stack" nav item; replaced mission copy with a platform-focused narrative and six core principles.
Analytics & Config
src/core/config.ts, src/core/analytics/authorship.ts, src/cli/commands/stats.tsx, src/index.ts
Enabled analytics by default; marked authorship module experimental (comment); clarified CLI analytics error text; removed AuthorshipLedger from analytics barrel export.
Heartbeat / Dispatcher / Daemon
src/core/dispatcher.ts, src/core/feeds/daemon-process.ts, src/core/feeds/daemon-manager.ts
Dispatcher now writes _dispatch_heartbeat; daemon writes/refreshes _daemon_heartbeat (90s TTL, 30s interval); daemon inactivity checks reference _dispatch_heartbeat; builtin feeds list expanded (added practice, weather, memories).
Feeds Producers & Insights
src/core/feeds/producers/insights.ts, src/core/feeds/producers/practice.ts
Peak-hour computation changed to convert UTC→local; practice producer docs simplified to a single practice segment.
Status-line Segments & Two-Tier Layout
src/core/status-line/segments.ts, src/core/status-line/two-tier.ts
Removed ai_ratio and practice_breadcrumb; added daemon_health and agents segments and agent data structures; TwoTier config adds middleSegments and showSeparator, places agents in middle and daemon_health in fixed segments; minor insights text tweaks.
TUI Launcher, PID & Config
src/core/tui-launcher.ts, tui/hookwise_tui/app.py, tui/hookwise_tui/data.py, tui/tests/test_data.py
Added pgrep-based duplicate detection for newWindow (execSync); TUI writes/removes PID file; added _effective_config_path and write_config for persisting config with tests.
TUI Tabs & Widgets
tui/hookwise_tui/tabs/analytics.py, tui/hookwise_tui/tabs/coaching.py, tui/hookwise_tui/tabs/status.py, tui/hookwise_tui/tabs/dashboard.py, tui/hookwise_tui/widgets/feature_card.py
Removed AI-authorship UI; coaching tab gains interactive toggles persisted via write_config and on_switch_changed; status tab refactored into FIXED/ROTATING/OTHER groups with SegmentRow and preview refresh; small dashboard CSS and card-title min-width change.
TUI Styling Removed
tui/hookwise_tui/app.tcss
Removed metric-related CSS blocks (metric-card, metric-value, metric-label, metric-grid).
Agent Tracker Hook & CLI Status Merge
hooks/agent-tracker.sh, src/cli/commands/status-line.ts
Added hooks/agent-tracker.sh to maintain active-agents.json; CLI status-line loads active agents into cache under agents before rendering.
Tests Updated (Core & Integration)
tests/... (multiple under tests/core, tests/integration, tui/tests)
Many tests updated to disable analytics by default, adopt renamed heartbeat keys (_heartbeat_dispatch_heartbeat/_daemon_heartbeat), adjust built-in feed counts and producer→segment mappings (removed practice_breadcrumb), and add agents segment tests; added launcher execSync mock and new test for pgrep skip.

Sequence Diagram(s)

sequenceDiagram
  participant Dispatcher
  participant Cache
  participant Daemon
  participant FeedLoop

  Note over Dispatcher,Cache: Dispatch heartbeat path
  Dispatcher->>Cache: writeMerge("_dispatch_heartbeat", {ts})
  Cache-->>Dispatcher: ack

  Note over Daemon,Cache: Daemon periodic heartbeat
  Daemon->>Cache: mergeKey("_daemon_heartbeat", {pid, ts}, ttl=90s)
  Cache-->>Daemon: ack
  Daemon->>FeedLoop: start feed intervals / monitor activity

  Note over Daemon,Cache: Inactivity check uses dispatch heartbeat
  Daemon->>Cache: read("_dispatch_heartbeat")
  Cache-->>Daemon: value or empty -> fallback daemonStartTime
  Daemon->>Daemon: determine stale/healthy, continue fail-open on errors
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 44.19% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main changes: bug fixes (7 v1.4 bugs) and removal of orphaned segments, which directly align with the PR's primary objectives.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/v1.4-bugfixes

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
tests/core/feeds/producers/insights.test.ts (1)

198-213: 🧹 Nitpick | 🔵 Trivial

Avoid mirroring production formula in the assertion

Line 211–Line 213 duplicates implementation math, which weakens the test. Prefer fixing timezone offset in-test (spy/mock) and asserting a constant expected hour.

✅ Example adjustment
-import { describe, it, expect, beforeEach, afterEach } from "vitest";
+import { describe, it, expect, beforeEach, afterEach, vi } from "vitest";
...
   it("computes peak_hour correctly (UTC→local conversion)", () => {
+    const tzSpy = vi
+      .spyOn(Date.prototype, "getTimezoneOffset")
+      .mockReturnValue(330); // UTC+05:30
+
     const usageDir = setupFixtures(
       tempRoot,
       ["fresh-clean.json", "fresh-friction.json"],
       [],
     );

     const result = aggregateInsights(usageDir, 30, NOW);
     expect(result).not.toBeNull();
-    const offsetMinutes = new Date().getTimezoneOffset();
-    const expectedLocalPeak = ((11 - offsetMinutes / 60) + 24) % 24;
-    expect(result!.peak_hour).toBe(expectedLocalPeak);
+    expect(result!.peak_hour).toBe(16);
+    tzSpy.mockRestore();
   });

As per coding guidelines: “Tests must test logic, not mock implementations. Flag tests where the mock setup is more complex than the assertion.”

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/core/feeds/producers/insights.test.ts` around lines 198 - 213, The test
duplicates the production timezone conversion logic when computing
expectedLocalPeak; instead, stub/spy the system timezone offset to a fixed value
(e.g. jest.spyOn(Date.prototype,
"getTimezoneOffset").mockReturnValue(fixedOffsetMinutes) or equivalent) before
calling aggregateInsights, then compute the expected local peak using that known
fixed offset and assert result!.peak_hour equals that constant; update the test
around aggregateInsights and the offset calculation to remove the mirrored
implementation and restore the spy after the test.
tests/core/feeds/segments.test.ts (1)

518-523: ⚠️ Potential issue | 🟠 Major

Missing explicit membership check for one of the 20 built-ins.

Line 528 asserts total length 20, but the expected-name list (Line 518 onward) contains only 19 entries. This can pass even if the wrong segment set is registered. Add the missing segment name (likely daemon_health) to make membership validation exact.

Proposed test fix
     const expected = [
       "clock", "mantra", "builder_trap", "session", "practice", "cost", "streak",
-      "context_bar", "mode_badge", "duration",
+      "context_bar", "mode_badge", "duration", "daemon_health",
       "pulse", "project", "calendar", "news",
       "insights_friction", "insights_pace", "insights_trend",
       "weather", "memories",
     ];

Also applies to: 528-528

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/core/feeds/segments.test.ts` around lines 518 - 523, The expected
built-ins list in tests/core/feeds/segments.test.ts is missing one entry (the
test asserts length 20 but the array contains 19), so add the missing segment
name "daemon_health" to the expected-names array used in the test (the array
starting with "clock", "mantra", ... "memories") so the membership check and
length assertion match the actual registered segments.
tests/core/dispatcher.test.ts (1)

790-799: 🧹 Nitpick | 🔵 Trivial

Inconsistent config usage: this test still uses getDefaultConfig() directly.

For consistency with the rest of the file, consider using makeConfig() here as well to disable analytics. While this particular test may not trigger analytics (invalid event type), maintaining consistency avoids potential flakiness and makes the pattern uniform.

🔧 Suggested fix
   it("safeDispatch wraps dispatch for fail-open", () => {
     // Pass a config that would normally work but force an internal error scenario
     const result = dispatch(
       "InvalidEvent" as EventType,
       makePayload(),
-      { config: getDefaultConfig() }
+      { config: makeConfig() }
     );
     expect(result.exitCode).toBe(0);
     expect(result.stdout).toBeNull();
   });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tests/core/dispatcher.test.ts` around lines 790 - 799, The test uses
getDefaultConfig() directly which is inconsistent with other tests; update the
call to use makeConfig() (the same helper used elsewhere) when constructing the
config passed to dispatch in the "safeDispatch wraps dispatch for fail-open"
test so analytics are disabled consistently—locate the test that calls
dispatch("InvalidEvent" as EventType, makePayload(), { config:
getDefaultConfig() }) and replace getDefaultConfig() with makeConfig().
tui/hookwise_tui/tabs/analytics.py (1)

43-99: ⚠️ Potential issue | 🟡 Minor

Remove orphaned authorship queries and update inaccurate feature description.

The read_analytics() function in tui/hookwise_tui/data.py still queries the authorship_ledger table and populates the AuthorshipSummary dataclass, but this data is never used in the analytics tab after removing the AI ratio visualization. The dashboard feature description in tui/hookwise_tui/tabs/dashboard.py (line 16) still mentions "AI authorship ratio" which is no longer displayed.

  • Remove the authorship query and AuthorshipSummary from data.py (lines 236-263)
  • Update the dashboard feature description to remove mention of "AI authorship ratio"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tui/hookwise_tui/tabs/analytics.py` around lines 43 - 99, The analytics code
still builds an unused AuthorshipSummary from the authorship_ledger in
read_analytics; remove the authorship query and the AuthorshipSummary population
inside read_analytics in tui/hookwise_tui/data.py (delete the block that queries
authorship_ledger and any construction/return of AuthorshipSummary), and update
any imports/typing that referenced AuthorshipSummary; then edit the dashboard
feature description string in tui/hookwise_tui/tabs/dashboard.py to remove the
phrase "AI authorship ratio" so the description only lists the remaining
features. Ensure no other code references AuthorshipSummary or the
authorship_ledger query after removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/core/feeds/daemon-process.ts`:
- Around line 303-306: The code assumes parsed._dispatch_heartbeat.value is a
number via a cast (heartbeat) but doesn't validate it; update the logic around
parsed/_dispatch_heartbeat/heartbeat/referenceTime so heartbeat is checked with
Number.isFinite (or typeof === 'number' and isFinite) and only used when valid
(and optionally within a sensible range), otherwise fall back to daemonStartTime
before computing Date.now() - referenceTime and using
config.daemon.inactivityTimeoutMinutes; ensure any downstream math never
receives NaN by performing this validation and assigning referenceTime
accordingly.
- Around line 259-261: Wrap the initial heartbeat cache write call in a
try/catch so a failing mergeKey(cachePath, "_daemon_heartbeat", { value:
Date.now() }, DAEMON_HEARTBEAT_TTL) cannot abort daemon startup; catch and log
the error (using the module's existing logger) and proceed to start the
refresh/timer logic regardless. Locate the Step 5 startup code around the
mergeKey invocation in daemon-process (the daemon startup/initialization
function) and replace the raw call with a guarded async/await call inside try {
await mergeKey(...) } catch (err) { logger.error(..., err) } so timers/intervals
are still established even if the cache write fails. Ensure behavior/return flow
is unchanged on success.

In `@src/core/feeds/producers/insights.ts`:
- Around line 205-206: The code currently computes offsetMinutes with new Date()
and divides by 60 producing fractional hours; replace new Date() with the
provided now parameter to be deterministic, compute an integer hour offset using
Math.trunc(offsetMinutes / 60) (or Math.floor/Math.trunc as appropriate for
negative values) instead of dividing to a float, and then calculate peakHour as
((peakHourUtc - offsetHour) + 24) % 24 so peakHour remains an integer hour;
update references to offsetMinutes, peakHourUtc, and peakHour in the function to
use now.getTimezoneOffset() and the integer offsetHour variable.

In `@src/core/status-line/segments.ts`:
- Line 456: The hardcoded staleThresholdMs = 90_000 in segments.ts duplicates
the daemon heartbeat TTL; replace the magic number with the shared constant
exported from the daemon heartbeat logic (e.g., import DAEMON_HEARTBEAT_TTL_MS
or HEARTBEAT_TTL_MS) and use that value (or a clearly derived value) for
staleThresholdMs; if the daemon module does not yet export a named TTL constant,
add an export (e.g., export const DAEMON_HEARTBEAT_TTL_MS = 90_000) in the
daemon code and import it into segments.ts so both places reference the same
symbol.

In `@tests/core/tui-launcher.test.ts`:
- Around line 205-219: The test currently uses
mockedExecSync.mockReturnValue("12345\n") and then resets it later; change this
to a one-shot mock (e.g., mockedExecSync.mockReturnValueOnce("12345\n") or
mockedExecSync.mockImplementationOnce(() => "12345\n")) so the execSync behavior
only applies for this call and you can remove the manual reset at the end;
update the assertion block around launchTui(config, pidPath) that references
mockedSpawn and keep references to mockedExecSync and launchTui for context.

In `@tui/hookwise_tui/data.py`:
- Around line 84-93: write_config currently always writes to
_default_config_path(), which can differ from the file read_config actually
reads; update write_config to use the same path-resolution logic as read_config
(i.e., if config_path is None, locate the effective config file the same way
read_config does—prefer any existing config file read_config checks and fall
back to the same _default_config_path()/~/.hookwise/config.yaml). In practice
either call the common resolver used by read_config or replicate its lookup
(iterate the same candidate paths, pick an existing one if present, otherwise
use _default_config_path()), then write to that resolved Path before returning
True.

In `@tui/hookwise_tui/tabs/coaching.py`:
- Around line 106-129: The FeatureCard visuals aren't updated after toggling
because on_switch_changed only persists config; after a successful
write_config(config) locate the mounted FeatureCard for the toggled feature (use
feature_key from switch_id, e.g. query_one or query for an element with an
id/class like f"feature-{feature_key}" and the FeatureCard class) and call its
update method(s) to reflect the new state (e.g. set an enabled property, call a
method like set_enabled/refresh/update_badge or update attributes/classes to
change badge and border) so the UI matches the persisted config immediately;
perform this update only after write_config returns true and keep the existing
notify logic.

In `@tui/hookwise_tui/tabs/status.py`:
- Around line 230-279: _refresh_preview currently only builds preview_text and
summary_lines from FIXED_SEGMENTS and ROTATING_SEGMENTS, so any toggled entries
in OTHER_SEGMENTS are omitted; modify _refresh_preview to also collect
other_active = [s for s in OTHER_SEGMENTS if s in active_set], render each via
self._render_segment(seg, cache) and include non-empty results in the live
preview (e.g., append them as separate lines after line1/line2 or as additional
lines in preview_text) and add a summary entry (e.g., "[bold]Other:[/bold] " +
delimiter.join(other_active) or a per-segment list) to summary_lines so
standalone segments appear in both the mounted preview (the
Static(preview_text,...)) and the tier summary.

---

Outside diff comments:
In `@tests/core/dispatcher.test.ts`:
- Around line 790-799: The test uses getDefaultConfig() directly which is
inconsistent with other tests; update the call to use makeConfig() (the same
helper used elsewhere) when constructing the config passed to dispatch in the
"safeDispatch wraps dispatch for fail-open" test so analytics are disabled
consistently—locate the test that calls dispatch("InvalidEvent" as EventType,
makePayload(), { config: getDefaultConfig() }) and replace getDefaultConfig()
with makeConfig().

In `@tests/core/feeds/producers/insights.test.ts`:
- Around line 198-213: The test duplicates the production timezone conversion
logic when computing expectedLocalPeak; instead, stub/spy the system timezone
offset to a fixed value (e.g. jest.spyOn(Date.prototype,
"getTimezoneOffset").mockReturnValue(fixedOffsetMinutes) or equivalent) before
calling aggregateInsights, then compute the expected local peak using that known
fixed offset and assert result!.peak_hour equals that constant; update the test
around aggregateInsights and the offset calculation to remove the mirrored
implementation and restore the spy after the test.

In `@tests/core/feeds/segments.test.ts`:
- Around line 518-523: The expected built-ins list in
tests/core/feeds/segments.test.ts is missing one entry (the test asserts length
20 but the array contains 19), so add the missing segment name "daemon_health"
to the expected-names array used in the test (the array starting with "clock",
"mantra", ... "memories") so the membership check and length assertion match the
actual registered segments.

In `@tui/hookwise_tui/tabs/analytics.py`:
- Around line 43-99: The analytics code still builds an unused AuthorshipSummary
from the authorship_ledger in read_analytics; remove the authorship query and
the AuthorshipSummary population inside read_analytics in
tui/hookwise_tui/data.py (delete the block that queries authorship_ledger and
any construction/return of AuthorshipSummary), and update any imports/typing
that referenced AuthorshipSummary; then edit the dashboard feature description
string in tui/hookwise_tui/tabs/dashboard.py to remove the phrase "AI authorship
ratio" so the description only lists the remaining features. Ensure no other
code references AuthorshipSummary or the authorship_ledger query after removal.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 6f61300c-f1c9-4cf6-b6b7-bec277b1defa

📥 Commits

Reviewing files that changed from the base of the PR and between 137ac5a and 60bc5f0.

📒 Files selected for processing (38)
  • docs/.vitepress/config.ts
  • docs/philosophy.md
  • src/cli/commands/stats.tsx
  • src/core/analytics/authorship.ts
  • src/core/config.ts
  • src/core/dispatcher.ts
  • src/core/feeds/daemon-manager.ts
  • src/core/feeds/daemon-process.ts
  • src/core/feeds/producers/insights.ts
  • src/core/feeds/producers/practice.ts
  • src/core/status-line/segments.ts
  • src/core/status-line/two-tier.ts
  • src/core/tui-launcher.ts
  • src/index.ts
  • tests/core/dispatcher.test.ts
  • tests/core/feeds/daemon-manager.test.ts
  • tests/core/feeds/daemon-process.test.ts
  • tests/core/feeds/dispatch-integration.test.ts
  • tests/core/feeds/producers/insights.test.ts
  • tests/core/feeds/segments.test.ts
  • tests/core/guard-contracts.test.ts
  • tests/core/status-line.test.ts
  • tests/core/status-line/insights-segments.test.ts
  • tests/core/status-line/segments-new.test.ts
  • tests/core/status-line/two-tier.test.ts
  • tests/core/tui-launcher.test.ts
  • tests/integration/dispatch-flow.test.ts
  • tests/integration/dispatcher-wiring.test.ts
  • tests/integration/pipeline-wiring.test.ts
  • tests/integration/pipeline.test.ts
  • tui/hookwise_tui/app.py
  • tui/hookwise_tui/app.tcss
  • tui/hookwise_tui/data.py
  • tui/hookwise_tui/tabs/analytics.py
  • tui/hookwise_tui/tabs/coaching.py
  • tui/hookwise_tui/tabs/dashboard.py
  • tui/hookwise_tui/tabs/status.py
  • tui/hookwise_tui/widgets/feature_card.py
💤 Files with no reviewable changes (2)
  • src/index.ts
  • tui/hookwise_tui/app.tcss

Comment thread src/core/feeds/daemon-process.ts
Comment thread src/core/feeds/daemon-process.ts Outdated
Comment thread src/core/feeds/producers/insights.ts Outdated
if (!heartbeat?.value) return "";

const ageMs = Date.now() - heartbeat.value;
const staleThresholdMs = 90_000; // 90 seconds

@coderabbitai coderabbitai Bot Mar 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Avoid duplicating daemon heartbeat staleness constants.

Line 456 hardcodes 90_000, while heartbeat TTL is separately defined in daemon process logic. Please use a shared constant source to prevent stale/healthy drift over time.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/core/status-line/segments.ts` at line 456, The hardcoded staleThresholdMs
= 90_000 in segments.ts duplicates the daemon heartbeat TTL; replace the magic
number with the shared constant exported from the daemon heartbeat logic (e.g.,
import DAEMON_HEARTBEAT_TTL_MS or HEARTBEAT_TTL_MS) and use that value (or a
clearly derived value) for staleThresholdMs; if the daemon module does not yet
export a named TTL constant, add an export (e.g., export const
DAEMON_HEARTBEAT_TTL_MS = 90_000) in the daemon code and import it into
segments.ts so both places reference the same symbol.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Acknowledged as a valid nitpick but deferring for now. The 90_000 value is stable and only referenced in two places — extracting a shared constant adds a cross-module import dependency for a single number that hasn't drifted. Will address if the heartbeat TTL ever changes.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vishnujayvel, fair enough — the reasoning is sound for a two-reference stable value. If the heartbeat TTL ever changes or a third reference appears, extracting a shared constant would be the right call. Happy to defer this for now.

Comment thread tests/core/tui-launcher.test.ts
Comment thread tui/hookwise_tui/data.py
Comment on lines +106 to +129
def on_switch_changed(self, event: Switch.Changed) -> None:
switch_id = event.switch.id or ""
if not switch_id.startswith("switch-"):
return
feature_key = switch_id.removeprefix("switch-")

config = read_config()
coaching = config.get("coaching", {})
if not isinstance(coaching, dict):
coaching = {}
config["coaching"] = coaching

feature_cfg = coaching.get(feature_key, {})
if not isinstance(feature_cfg, dict):
feature_cfg = {}
feature_cfg["enabled"] = event.value
coaching[feature_key] = feature_cfg
config["coaching"] = coaching

if write_config(config):
state = "enabled" if event.value else "disabled"
self.notify(f"{feature_key} {state}")
else:
self.notify("Failed to save config", severity="error")

@coderabbitai coderabbitai Bot Mar 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Switch changes don’t update the paired FeatureCard state.

on_switch_changed() persists config, but the already-mounted FeatureCard keeps its original enabled/disabled visuals (badge + border). After a toggle, the row can show conflicting states until the tab is rebuilt.

💡 Minimal fix direction
         if write_config(config):
             state = "enabled" if event.value else "disabled"
             self.notify(f"{feature_key} {state}")
+            # Rebuild rows so FeatureCard visuals reflect the new value.
+            self.refresh(recompose=True)
         else:
             self.notify("Failed to save config", severity="error")
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tui/hookwise_tui/tabs/coaching.py` around lines 106 - 129, The FeatureCard
visuals aren't updated after toggling because on_switch_changed only persists
config; after a successful write_config(config) locate the mounted FeatureCard
for the toggled feature (use feature_key from switch_id, e.g. query_one or query
for an element with an id/class like f"feature-{feature_key}" and the
FeatureCard class) and call its update method(s) to reflect the new state (e.g.
set an enabled property, call a method like set_enabled/refresh/update_badge or
update attributes/classes to change badge and border) so the UI matches the
persisted config immediately; perform this update only after write_config
returns true and keep the existing notify logic.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferring this one. The FeatureCard visual state is purely cosmetic (badge color and border) and the underlying config is correctly persisted. A refresh(recompose=True) would rebuild the entire tab widget tree on every toggle, which is heavy for a visual-only sync. This needs a lighter approach (targeted CSS class update on the specific card) and proper Textual snapshot testing to verify. Tracking for a future iteration.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vishnujayvel, understood — agreed that a full recompose is overkill for a badge/border-only sync. The targeted approach (toggling a CSS class like .enabled/.disabled on the specific FeatureCard instance via query_one) will be much cleaner and avoids rebuilding the entire widget tree. Happy to help draft that when you're ready to address it, along with the Textual snapshot test setup.

(´• ω •`) 🐇


✏️ Learnings added
Learnt from: vishnujayvel
Repo: vishnujayvel/hookwise PR: 16
File: tui/hookwise_tui/tabs/coaching.py:106-129
Timestamp: 2026-03-05T22:28:46.998Z
Learning: In `tui/hookwise_tui/tabs/coaching.py`, the preferred approach to update `FeatureCard` visual state (badge color and border) after a switch toggle is a targeted CSS class update on the specific card instance (e.g., via `query_one`), NOT `refresh(recompose=True)` which rebuilds the entire tab widget tree. The fix should also be accompanied by Textual snapshot tests.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Comment thread tui/hookwise_tui/tabs/status.py
Vishnu Jayavel and others added 2 commits March 5, 2026 14:25
- hooks/agent-tracker.sh: SubagentStart/Stop state tracker with atomic writes
- agents segment: reads active-agents.json, renders colored tree with team info
- N-tier renderer: middleSegments + separator between fixed and rotating lines
- CLI: reads active-agents.json into cache for the agents segment
- Tests: 14 new tests (9 segment + 5 N-tier)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- CR-1: Guard initial daemon heartbeat write with try/catch (daemon-process.ts)
- CR-2: Validate heartbeat value with typeof + Number.isFinite (daemon-process.ts)
- CR-3: Fix fractional peak_hour for UTC+05:30/+05:45 timezones (insights.ts)
- CR-4: Use mockReturnValueOnce to avoid shared-state leak (tui-launcher.test.ts)
- CR-5: Unify read/write config path via _effective_config_path (data.py)
- CR-6: Include OTHER_SEGMENTS in status tab preview and summary (status.py)

Adds 7 Python tests for write_config and _effective_config_path.
Updates insights test to use minute-based formula matching production code.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment thread tui/hookwise_tui/data.py

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/core/status-line/segments.ts (1)

559-581: ⚠️ Potential issue | 🟠 Major

BUILTIN_SEGMENTS currently contains 21 entries; expected target is 20.

This appears to violate the stated objective of exact parity with the 20-segment TUI list.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/core/status-line/segments.ts` around lines 559 - 581, BUILTIN_SEGMENTS
currently lists 21 keys but should match the 20-segment TUI list exactly;
reconcile the arrays by comparing BUILTIN_SEGMENTS to the canonical TUI segment
list and remove the extra entry from the record so the map contains exactly the
20 expected keys. Update the BUILTIN_SEGMENTS object (and any corresponding
imports/exports for the segment symbol you remove) so it only includes the
official segment identifiers (leave the SegmentRenderer type intact) and run
tests to confirm parity.
♻️ Duplicate comments (1)
src/core/status-line/segments.ts (1)

545-547: ⚠️ Potential issue | 🟡 Minor

Use a shared daemon heartbeat TTL constant.

Line 546 hardcodes 90_000; this can drift from daemon heartbeat TTL logic and misclassify health over time.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/core/status-line/segments.ts` around lines 545 - 547, Replace the
hardcoded staleThresholdMs = 90_000 in src/core/status-line/segments.ts with a
shared daemon heartbeat TTL constant so health calculation stays in sync; import
the canonical TTL (e.g., DAEMON_HEARTBEAT_TTL_MS or HEARTBEAT_TTL_MS) from the
daemon/heartbeat or daemon/constants module and use that instead of 90_000 where
ageMs = Date.now() - heartbeat.value is compared to staleThresholdMs (update any
local variable name like staleThresholdMs to use the imported constant).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@hooks/agent-tracker.sh`:
- Around line 42-47: The current read-modify-write around STATE_FILE using
CURRENT is racy—wrap both the read and write when handling
SubagentStart/SubagentStop so updates cannot interleave; acquire an exclusive
lock (e.g., using flock on a dedicated lockfile or mkdir-based lock) before
reading CURRENT, perform the modify, then atomically write the updated
STATE_FILE and release the lock; ensure the same locking is applied to the other
block that handles state (the similar logic referenced around the later
SubagentStart/SubagentStop handling) so all accesses to STATE_FILE use the same
lock identifier.
- Around line 50-62: The inline python block assigning UPDATED is
injection-prone because it interpolates shell variables directly into the -c
string; instead invoke python3 with no source interpolation by passing the JSON
payload via stdin (read with json.loads(sys.stdin.read())) and pass scalars
(EVENT, agent_id, name, NOW, STALE_SECONDS, team_name, strategy) as safe argv
values or environment variables (access via sys.argv or os.environ) so you avoid
embedding unescaped values; update the python3 -c call that produces UPDATED to
read CURRENT from stdin and read other inputs from sys.argv (and apply the
fallback logic for team_name/strategy using current.get(...) inside the Python
code) to eliminate quote/newline injection risks.
- Line 7: The script currently uses the strict failure flag "set -euo pipefail"
which breaks the intended “always exits 0” fail-open contract; change it to
remove -e (e.g., "set -uo pipefail") and make potential-failing operations
(notably mkdir -p, temp file writes, mv and the block around lines 93-95)
resilient by either guarding them (if ...; then ...; fi) or appending "|| true"
so failures do not cause an exit, and ensure the script ends with an explicit
"exit 0" to guarantee a zero exit code; look for the literal "set -euo
pipefail", the mkdir/mv/temp write calls, and the block around lines 93-95 to
apply these changes.

In `@src/cli/commands/status-line.ts`:
- Line 18: ACTIVE_AGENTS_PATH is hardcoded to ~/.hookwise which diverges from
hooks/agent-tracker.sh that writes to HOOKWISE_STATE_DIR; update the code that
defines ACTIVE_AGENTS_PATH (const ACTIVE_AGENTS_PATH) to resolve the state
directory using the same logic the rest of the app/hooks use (check
process.env.HOOKWISE_STATE_DIR and fall back to join(homedir(), ".hookwise") or
reuse any existing resolveStateDir/getStateDir helper), and apply the same fix
to the related code around the block referenced at 82-85 so both reads/writes
target the same directory as the hook.

In `@src/core/status-line/segments.ts`:
- Around line 8-10: The agents segment currently performs synchronous disk I/O
during render (uses existsSync/readFileSync/join/homedir); remove those calls
from the render path in the agents segment and make it read only from the cache
bus (the cache key used for agents data). If the cache key is missing or stale,
return a sensible fallback (e.g., "no agents" or a placeholder string) rather
than performing I/O or returning empty string. Move any file reading logic into
the cache-population/updater code (so functions that write to the cache perform
existsSync/readFileSync and set the agents cache key), and ensure the agents
renderer function (agents in segments.ts) only reads the cache and formats the
value.
- Around line 539-544: The daemon_health SegmentRenderer currently returns an
empty string when cache._daemon_heartbeat or heartbeat.value is missing; instead
return a sensible fallback status string (e.g., a clear "daemon: unknown" or
"daemon: no heartbeat/stale") so the daemon state is visible; update the
daemon_health function to detect missing or stale heartbeat
(cache._daemon_heartbeat and heartbeat.value) and return that explicit fallback
rather than "".
- Around line 486-491: The current staleness filter uses started_at
(activeAgents: data.agents.filter(...) comparing now - started_at <
staleThreshold) which wrongly excludes legitimate long-running agents; change
the logic to base staleness on a heartbeat/last-seen timestamp (e.g.,
last_heartbeat, last_seen, or updated_at) if available on the agent object and
only filter out agents whose last_heartbeat is older than staleThreshold,
falling back to not filtering by started_at if no heartbeat field exists, and
keep the variable names staleThreshold and activeAgents but update the predicate
to use the heartbeat/last-seen field (or agent.status) instead of started_at.

In `@src/core/status-line/two-tier.ts`:
- Around line 21-24: Update the TwoTierConfig interface to make middleSegments
and showSeparator optional to match the implementation: change the property
declarations for middleSegments and showSeparator on TwoTierConfig so they
accept undefined (e.g., add ? to middleSegments and showSeparator) because the
code already uses middleSegments ?? [] and showSeparator ?? true in
renderTwoTierSections and buildTwoTierSeparator.

In `@tui/hookwise_tui/data.py`:
- Around line 92-101: write_config currently writes directly to the target file
and may produce truncated configs on interruption; change it to perform an
atomic write by writing the YAML to a temporary file in the same directory (use
the same parent from _effective_config_path(path) / Path.parent), flush and
fsync the temp file, close it, then atomically replace the target with
os.replace (or Path.rename) so the switch is atomic; preserve the existing
return True/False behavior and catch exceptions around the write/replace steps,
cleaning up the temp file on failure.

In `@tui/hookwise_tui/tabs/status.py`:
- Around line 322-326: The saved toggles are written to
config["status_line"]["segments"] but the CLI renderer still uses
renderTwoTier(DEFAULT_TWO_TIER_CONFIG, cache), so persisted toggles have no
effect; update the CLI renderer (src/cli/commands/status-line.ts) to load the
persisted config (status_line.segments) and merge or replace
DEFAULT_TWO_TIER_CONFIG with those segments before calling renderTwoTier (or
alternatively read the same cache key you persist to), ensuring renderTwoTier
receives the persisted segments instead of always using DEFAULT_TWO_TIER_CONFIG.

---

Outside diff comments:
In `@src/core/status-line/segments.ts`:
- Around line 559-581: BUILTIN_SEGMENTS currently lists 21 keys but should match
the 20-segment TUI list exactly; reconcile the arrays by comparing
BUILTIN_SEGMENTS to the canonical TUI segment list and remove the extra entry
from the record so the map contains exactly the 20 expected keys. Update the
BUILTIN_SEGMENTS object (and any corresponding imports/exports for the segment
symbol you remove) so it only includes the official segment identifiers (leave
the SegmentRenderer type intact) and run tests to confirm parity.

---

Duplicate comments:
In `@src/core/status-line/segments.ts`:
- Around line 545-547: Replace the hardcoded staleThresholdMs = 90_000 in
src/core/status-line/segments.ts with a shared daemon heartbeat TTL constant so
health calculation stays in sync; import the canonical TTL (e.g.,
DAEMON_HEARTBEAT_TTL_MS or HEARTBEAT_TTL_MS) from the daemon/heartbeat or
daemon/constants module and use that instead of 90_000 where ageMs = Date.now()
- heartbeat.value is compared to staleThresholdMs (update any local variable
name like staleThresholdMs to use the imported constant).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9705dc86-280b-4d6d-a413-d08fb715fa7c

📥 Commits

Reviewing files that changed from the base of the PR and between 60bc5f0 and 768a5e0.

📒 Files selected for processing (14)
  • hooks/agent-tracker.sh
  • src/cli/commands/status-line.ts
  • src/core/feeds/daemon-process.ts
  • src/core/feeds/producers/insights.ts
  • src/core/status-line/segments.ts
  • src/core/status-line/two-tier.ts
  • tests/core/feeds/producers/insights.test.ts
  • tests/core/status-line/agents-segment.test.ts
  • tests/core/status-line/segments-new.test.ts
  • tests/core/status-line/two-tier.test.ts
  • tests/core/tui-launcher.test.ts
  • tui/hookwise_tui/data.py
  • tui/hookwise_tui/tabs/status.py
  • tui/tests/test_data.py

Comment thread hooks/agent-tracker.sh Outdated
# Claude Code hook for SubagentStart / SubagentStop events.
# Reads JSON from stdin, maintains state at ~/.hookwise/cache/active-agents.json.
# Always exits 0 (fail-open).
set -euo pipefail

@coderabbitai coderabbitai Bot Mar 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

set -e currently breaks the “always exits 0” fail-open contract.

mkdir -p, temp write, or mv failures will terminate with non-zero exit, which contradicts the script contract in the header.

💡 Proposed hardening
-mkdir -p "$CACHE_DIR"
+mkdir -p "$CACHE_DIR" || exit 0
...
-echo "$UPDATED" > "$TMPFILE"
-mv "$TMPFILE" "$STATE_FILE"
+echo "$UPDATED" > "$TMPFILE" 2>/dev/null || exit 0
+mv "$TMPFILE" "$STATE_FILE" 2>/dev/null || { rm -f "$TMPFILE"; exit 0; }

Also applies to: 14-14, 93-95

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@hooks/agent-tracker.sh` at line 7, The script currently uses the strict
failure flag "set -euo pipefail" which breaks the intended “always exits 0”
fail-open contract; change it to remove -e (e.g., "set -uo pipefail") and make
potential-failing operations (notably mkdir -p, temp file writes, mv and the
block around lines 93-95) resilient by either guarding them (if ...; then ...;
fi) or appending "|| true" so failures do not cause an exit, and ensure the
script ends with an explicit "exit 0" to guarantee a zero exit code; look for
the literal "set -euo pipefail", the mkdir/mv/temp write calls, and the block
around lines 93-95 to apply these changes.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferring. Claude Code serializes hook invocations — SubagentStart/Stop events don't fire concurrently for the same session. The atomic rename already protects the write side. Race conditions would require two separate Claude Code sessions running hooks simultaneously on the same state file, which is an edge case we can address if it surfaces.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vishnujayvel — your point about concurrency is well-taken: serialized invocations + the atomic rename (mktempmv) together already eliminate the race-condition concern, and I agree that's a non-issue.

The original concern is actually orthogonal to concurrency. set -e will terminate the script with a non-zero exit code whenever any command fails — disk full, a permission error on mkdir -p, or an mv across filesystems — regardless of whether another session is involved. That's what breaks the "always exits 0" fail-open contract stated in the header: a transient I/O error would cause Claude Code to treat the hook as failed.

The minimal fix for that specific issue is just:

-set -euo pipefail
+set -uo pipefail

…and appending exit 0 at the end, without touching any concurrency-related logic. Happy to defer the guard-per-command hardening if you'd rather keep it simple for now, but removing -e alone would honour the contract the header promises.

Comment thread hooks/agent-tracker.sh
Comment on lines +42 to +47
# Read existing state (or start fresh)
if [ -f "$STATE_FILE" ]; then
CURRENT=$(cat "$STATE_FILE" 2>/dev/null || echo '{"agents":[],"team_name":"","strategy":""}')
else
CURRENT='{"agents":[],"team_name":"","strategy":""}'
fi

@coderabbitai coderabbitai Bot Mar 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add a lock around read-modify-write to prevent lost updates.

Concurrent SubagentStart/SubagentStop hooks can race: both read old state, then last writer wins and drops the other update.

Also applies to: 91-95

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@hooks/agent-tracker.sh` around lines 42 - 47, The current read-modify-write
around STATE_FILE using CURRENT is racy—wrap both the read and write when
handling SubagentStart/SubagentStop so updates cannot interleave; acquire an
exclusive lock (e.g., using flock on a dedicated lockfile or mkdir-based lock)
before reading CURRENT, perform the modify, then atomically write the updated
STATE_FILE and release the lock; ensure the same locking is applied to the other
block that handles state (the similar logic referenced around the later
SubagentStart/SubagentStop handling) so all accesses to STATE_FILE use the same
lock identifier.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferring. The values interpolated into the Python block come from Claude Code's hook event JSON payload — a trusted internal source, not user input. The agent_id, name, etc. are system-generated identifiers. Agreed that stdin-based passing would be more robust, but the injection risk here is theoretical given the trusted source.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vishnujayvel, just a quick clarification — my original concern was specifically about concurrent execution race conditions (two Claude Code hook processes running simultaneously, both reading the old STATE_FILE before either has written back, so one update silently overwrites the other), rather than injection risk from the payload values.

Your point about the trusted source is valid for the injection angle, but the race condition is independent of data trust — it's a filesystem concurrency issue. That said, since you're deferring, understood! If you open a follow-up issue later, the fix would be something like:

LOCK_FILE="$CACHE_DIR/.agent-tracker.lock"
(
  flock -x 200
  # ... read CURRENT, modify, write STATE_FILE atomically ...
) 200>"$LOCK_FILE"

No action needed from your side right now.

Comment thread hooks/agent-tracker.sh
Comment on lines +50 to +62
UPDATED=$(python3 -c "
import json, sys

current = json.loads('''$CURRENT''')
agents = current.get('agents', [])
event = '$EVENT'
agent_id = '$agent_id'
name = '$name'
now = $NOW
stale = $STALE_SECONDS
team_name = '$team_name' or current.get('team_name', '')
strategy = '$strategy' or current.get('strategy', '')

@coderabbitai coderabbitai Bot Mar 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Inline Python interpolation is injection-prone and brittle.

Unescaped values ($CURRENT, $agent_id, $team_name, etc.) are embedded directly into Python code. A quote/newline in input can break parsing or execute unintended Python statements.

🔒 Safer pattern (pass data as argv/stdin, not source interpolation)
-UPDATED=$(python3 -c "
-import json, sys
-current = json.loads('''$CURRENT''')
-event = '$EVENT'
-agent_id = '$agent_id'
-name = '$name'
-now = $NOW
-stale = $STALE_SECONDS
-team_name = '$team_name' or current.get('team_name', '')
-strategy = '$strategy' or current.get('strategy', '')
-...
-print(json.dumps(result, indent=2))
-" 2>/dev/null) || exit 0
+UPDATED=$(
+python3 - "$CURRENT" "$EVENT" "$agent_id" "$name" "$NOW" "$STALE_SECONDS" "$team_name" "$strategy" <<'PY'
+import json, sys
+current_json, event, agent_id, name, now_s, stale_s, team_name, strategy = sys.argv[1:]
+current = json.loads(current_json) if current_json else {"agents": [], "team_name": "", "strategy": ""}
+now = int(now_s)
+stale = int(stale_s)
+agents = current.get("agents", [])
+team_name = team_name or current.get("team_name", "")
+strategy = strategy or current.get("strategy", "")
+# ...existing mutation logic...
+print(json.dumps({"agents": agents, "team_name": team_name, "strategy": strategy, "updated_at": now}, indent=2))
+PY
+) 2>/dev/null || exit 0

Also applies to: 66-89

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@hooks/agent-tracker.sh` around lines 50 - 62, The inline python block
assigning UPDATED is injection-prone because it interpolates shell variables
directly into the -c string; instead invoke python3 with no source interpolation
by passing the JSON payload via stdin (read with json.loads(sys.stdin.read()))
and pass scalars (EVENT, agent_id, name, NOW, STALE_SECONDS, team_name,
strategy) as safe argv values or environment variables (access via sys.argv or
os.environ) so you avoid embedding unescaped values; update the python3 -c call
that produces UPDATED to read CURRENT from stdin and read other inputs from
sys.argv (and apply the fallback logic for team_name/strategy using
current.get(...) inside the Python code) to eliminate quote/newline injection
risks.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6cf685d. Changed set -euo pipefail to set -uo pipefail — the script already has || exit 0 and 2>/dev/null guards on failure-prone operations, and ends with explicit exit 0. Removing -e ensures intermediate failures don't short-circuit the fail-open contract.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

Let me check the actual commit to verify what was changed.


🏁 Script executed:

#!/bin/bash
gh api repos/vishnujayvel/hookwise/commits/6cf685d --jq '.sha, .commit.message' 2>/dev/null || echo "not found"
gh api repos/vishnujayvel/hookwise/commits/6cf685d/files --jq '.[].patch' 2>/dev/null | head -100

Length of output: 431


🏁 Script executed:

#!/bin/bash
# Try to get the diff via compare API or PR commits
gh api repos/vishnujayvel/hookwise/pulls/16/commits --jq '.[].sha + " " + .[].commit.message' 2>/dev/null | tail -5

# Try to view current state of the file
gh api repos/vishnujayvel/hookwise/contents/hooks/agent-tracker.sh --jq '.content' 2>/dev/null | base64 -d | head -70

Length of output: 2594


@vishnujayvel, thanks for the set -e fix — that does correctly preserve the fail-open contract. However, the original finding I raised is still present in the current file.

The set -e removal was a separate concern. My comment was specifically about the UPDATED=$(python3 -c "...") block where shell variables are interpolated directly into Python source code:

# Still in the current file (lines ~50+):
UPDATED=$(python3 -c "
current = json.loads('''$CURRENT''')   # $CURRENT is a JSON blob — breakable with ''' in any value
event = '$EVENT'
agent_id = '$agent_id'                  # single-quote in agent_id breaks Python syntax
name = '$name'
team_name = '$team_name' or ...
strategy = '$strategy' or ...
")

Why this is still risky: $CURRENT is read from disk (trusted), but $agent_id, $name, $team_name, and $strategy are extracted from Claude Code hook input, which could contain quotes or newlines from an agent's working directory path (e.g. a cwd with a ' in it). A path like /Users/alice/it's-a-project would break the Python -c string.

The safer fix from my original suggestion — passing values via sys.argv or a heredoc instead of source interpolation — is still worth applying:

UPDATED=$(
python3 - "$CURRENT" "$EVENT" "$agent_id" "$name" "$NOW" "$STALE_SECONDS" "$team_name" "$strategy" <<'PY'
import json, sys
current_json, event, agent_id, name, now_s, stale_s, team_name, strategy = sys.argv[1:]
current = json.loads(current_json) if current_json else {"agents": [], "team_name": "", "strategy": ""}
now = int(now_s)
stale = int(stale_s)
agents = current.get("agents", [])
team_name = team_name or current.get("team_name", "")
strategy = strategy or current.get("strategy", "")
# ... rest of mutation logic unchanged ...
PY
) 2>/dev/null || true

This pattern is immune to quote injection because values never touch Python source — they're passed as process arguments and read via sys.argv.

Comment thread src/cli/commands/status-line.ts
Comment on lines +8 to +10
import { existsSync, readFileSync } from "node:fs";
import { join } from "node:path";
import { homedir } from "node:os";

@coderabbitai coderabbitai Bot Mar 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Keep agents renderer cache-only; remove disk reads from render path.

agents currently performs synchronous file I/O (existsSync/readFileSync) during segment rendering. This breaks renderer purity and can block status-line rendering.

💡 Proposed refactor
-import { existsSync, readFileSync } from "node:fs";
-import { join } from "node:path";
-import { homedir } from "node:os";
@@
-const AGENTS_CACHE_PATH = join(homedir(), ".hookwise", "cache", "active-agents.json");
-
 const agents: SegmentRenderer = (cache) => {
-  // Read from cache.agents (merged by CLI command) or directly from file
-  let data: ActiveAgentsData | undefined = cache.agents as ActiveAgentsData | undefined;
-
-  if (!data) {
-    try {
-      if (!existsSync(AGENTS_CACHE_PATH)) return "";
-      const raw = readFileSync(AGENTS_CACHE_PATH, "utf-8");
-      data = JSON.parse(raw) as ActiveAgentsData;
-    } catch {
-      return "";
-    }
-  }
-
-  if (!data?.agents?.length) return "";
+  const data = cache.agents as ActiveAgentsData | undefined;
+  if (!data?.agents?.length) return "agents --";

As per coding guidelines: "Segments are pure functions of cache bus data — no direct I/O." and "Missing or stale cache data produces a sensible fallback, not an error or empty string."

Also applies to: 468-482, 484-485

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/core/status-line/segments.ts` around lines 8 - 10, The agents segment
currently performs synchronous disk I/O during render (uses
existsSync/readFileSync/join/homedir); remove those calls from the render path
in the agents segment and make it read only from the cache bus (the cache key
used for agents data). If the cache key is missing or stale, return a sensible
fallback (e.g., "no agents" or a placeholder string) rather than performing I/O
or returning empty string. Move any file reading logic into the
cache-population/updater code (so functions that write to the cache perform
existsSync/readFileSync and set the agents cache key), and ensure the agents
renderer function (agents in segments.ts) only reads the cache and formats the
value.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By design. The CLI command in status-line.ts pre-loads active-agents.json into cache.agents before rendering, so the disk I/O fallback in the segment renderer only fires if the cache wasn't pre-populated (e.g., direct segment unit testing). This matches the pattern used by other segments — renderers should work standalone too, not only when a cache-population layer exists upstream.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rate Limit Exceeded

@vishnujayvel have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 22 seconds before sending another message.

Comment on lines +486 to +491
// Filter stale entries (older than 10 minutes)
const now = Math.floor(Date.now() / 1000);
const staleThreshold = 600;
const activeAgents = data.agents.filter(
(a) => (now - (a.started_at ?? now)) < staleThreshold
);

@coderabbitai coderabbitai Bot Mar 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

started_at staleness logic drops valid long-running agents.

Filtering by now - started_at < 600 removes agents that are still active but older than 10 minutes.

💡 Proposed fix
   const now = Math.floor(Date.now() / 1000);
   const staleThreshold = 600;
-  const activeAgents = data.agents.filter(
-    (a) => (now - (a.started_at ?? now)) < staleThreshold
-  );
+  if (data.updated_at && (now - data.updated_at) > staleThreshold) {
+    return "agents stale";
+  }
+  const activeAgents = data.agents;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Filter stale entries (older than 10 minutes)
const now = Math.floor(Date.now() / 1000);
const staleThreshold = 600;
const activeAgents = data.agents.filter(
(a) => (now - (a.started_at ?? now)) < staleThreshold
);
// Filter stale entries (older than 10 minutes)
const now = Math.floor(Date.now() / 1000);
const staleThreshold = 600;
if (data.updated_at && (now - data.updated_at) > staleThreshold) {
return "agents stale";
}
const activeAgents = data.agents;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/core/status-line/segments.ts` around lines 486 - 491, The current
staleness filter uses started_at (activeAgents: data.agents.filter(...)
comparing now - started_at < staleThreshold) which wrongly excludes legitimate
long-running agents; change the logic to base staleness on a heartbeat/last-seen
timestamp (e.g., last_heartbeat, last_seen, or updated_at) if available on the
agent object and only filter out agents whose last_heartbeat is older than
staleThreshold, falling back to not filtering by started_at if no heartbeat
field exists, and keep the variable names staleThreshold and activeAgents but
update the predicate to use the heartbeat/last-seen field (or agent.status)
instead of started_at.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intentional. daemon_health returning "" when no heartbeat exists hides the segment entirely — which is correct behavior when the daemon isn't running. Showing "daemon: unknown" would add noise to the status line for users who don't use the daemon. The segment appears automatically once the daemon starts writing heartbeats.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vishnujayvel, thanks for the context — the daemon_health returning "" behaviour is completely correct and I wasn't questioning that.

However, my review comment is about a different segment: the agents segment's staleness filter on Lines 486–491:

const activeAgents = data.agents.filter(
  (a) => (now - (a.started_at ?? now)) < staleThreshold  // 600 s = 10 min
);

The issue here is that this compares now against started_at (the moment the sub-agent was spawned). A sub-agent that has been legitimately running for more than 10 minutes — e.g., a long code review, a compilation task, or a deep refactor — will be silently removed from the rendered list even though it is still active. The agent-tracker.sh script writes started_at once at spawn and does not update it while the agent is alive, so this filter is anchored to the wrong timestamp.

A more robust approach would be to gate on data.updated_at (which reflects the last write by the tracker script) rather than each agent's started_at:

const now = Math.floor(Date.now() / 1000);
const staleThreshold = 600;

// If the tracker file itself hasn't been updated in 10 min, the whole dataset is stale
if (data.updated_at && (now - data.updated_at) > staleThreshold) {
  return "agents stale";
}
const activeAgents = data.agents;

This way, long-running agents stay visible as long as the tracker is writing heartbeats, and the whole block disappears only when the tracker itself has gone silent. Would you like to address this, or is there a reason started_at is intentional here too?

Comment on lines +539 to +544
const daemon_health: SegmentRenderer = (cache) => {
const heartbeat = cache._daemon_heartbeat as
| { value?: number }
| undefined;
if (!heartbeat?.value) return "";

@coderabbitai coderabbitai Bot Mar 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Return explicit fallback when daemon heartbeat is missing.

Current behavior returns empty output and hides daemon state entirely.

💡 Proposed fix
 const daemon_health: SegmentRenderer = (cache) => {
   const heartbeat = cache._daemon_heartbeat as
     | { value?: number }
     | undefined;
-  if (!heartbeat?.value) return "";
+  if (heartbeat?.value == null) return "daemon --";

As per coding guidelines: "Missing or stale cache data produces a sensible fallback, not an error or empty string."

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const daemon_health: SegmentRenderer = (cache) => {
const heartbeat = cache._daemon_heartbeat as
| { value?: number }
| undefined;
if (!heartbeat?.value) return "";
const daemon_health: SegmentRenderer = (cache) => {
const heartbeat = cache._daemon_heartbeat as
| { value?: number }
| undefined;
if (heartbeat?.value == null) return "daemon --";
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/core/status-line/segments.ts` around lines 539 - 544, The daemon_health
SegmentRenderer currently returns an empty string when cache._daemon_heartbeat
or heartbeat.value is missing; instead return a sensible fallback status string
(e.g., a clear "daemon: unknown" or "daemon: no heartbeat/stale") so the daemon
state is visible; update the daemon_health function to detect missing or stale
heartbeat (cache._daemon_heartbeat and heartbeat.value) and return that explicit
fallback rather than "".

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The staleness filter using started_at is correct for our agent model. Agents don't have heartbeats — they have a start event and a stop event. The 10-minute threshold catches orphaned entries where SubagentStop was never received (e.g., crash). A long-running agent that's still active will have its started_at refreshed if it re-dispatches, and the agent-tracker hook cleans entries on every invocation.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vishnujayvel — Thanks for the explanation! The agent staleness model makes complete sense — start/stop events with a 10-minute orphan window and agent-tracker cleanup on every invocation is a clean design that doesn't need per-agent heartbeats.

However, my original comment was specifically about the daemon_health segment (not the agents staleness filter). The concern there is still open:

const daemon_health: SegmentRenderer = (cache) => {
  const heartbeat = cache._daemon_heartbeat as
    | { value?: number }
    | undefined;
  if (!heartbeat?.value) return "";  // ← this case

When _daemon_heartbeat is absent from the cache entirely — e.g., the daemon has never started, or the cache file was cleared — the segment silently returns "", so the status line shows nothing about daemon state. Per the coding guidelines for src/core/status-line/**: "Missing or stale cache data produces a sensible fallback, not an error or empty string."

The proposed fix applies specifically here:

- if (!heartbeat?.value) return "";
+ if (heartbeat?.value == null) return "daemon --";

This way, "daemon --" is shown when the key is absent (daemon not running or cache cold), while the existing stale-detection path (ageMs > staleThresholdMs) handles the "daemon ran but stopped" case with "daemon stale (Xm)". Would you like to address this one?


✏️ Learnings added
Learnt from: vishnujayvel
Repo: vishnujayvel/hookwise PR: 16
File: src/core/status-line/segments.ts:539-544
Timestamp: 2026-03-05T22:42:12.628Z
Learning: In hookwise, agents in src/core/status-line/segments.ts do not have heartbeats — they use a start/stop event model (SubagentStart/SubagentStop). The 10-minute stale threshold on `started_at` is intentional to catch orphaned entries where SubagentStop was never received (e.g., crash). The agent-tracker hook (hooks/agent-tracker.sh) cleans entries on every invocation, and long-running active agents have their `started_at` refreshed on re-dispatch.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Comment thread src/core/status-line/two-tier.ts Outdated
Comment thread tui/hookwise_tui/data.py
Comment on lines +92 to +101
def write_config(config: dict[str, Any], config_path: Path | None = None) -> bool:
"""Write hookwise.yaml config. Returns True on success."""
path = _effective_config_path(config_path)
try:
path.parent.mkdir(parents=True, exist_ok=True)
with open(path, "w") as f:
yaml.dump(config, f, default_flow_style=False, sort_keys=False)
return True
except Exception:
return False

@coderabbitai coderabbitai Bot Mar 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Use atomic writes to avoid config corruption on interruption/concurrent writes.

write_config() writes directly to the target file. If the process exits mid-write, users can end up with a truncated YAML file and broken toggles persistence.

💡 Proposed fix (atomic temp file + replace)
@@
 import json
 import os
 import sqlite3
+import tempfile
@@
 def write_config(config: dict[str, Any], config_path: Path | None = None) -> bool:
     """Write hookwise.yaml config. Returns True on success."""
     path = _effective_config_path(config_path)
+    tmp_path: Path | None = None
     try:
         path.parent.mkdir(parents=True, exist_ok=True)
-        with open(path, "w") as f:
-            yaml.dump(config, f, default_flow_style=False, sort_keys=False)
+        with tempfile.NamedTemporaryFile(
+            mode="w",
+            encoding="utf-8",
+            dir=path.parent,
+            delete=False,
+        ) as tmp:
+            yaml.safe_dump(config, tmp, default_flow_style=False, sort_keys=False)
+            tmp_path = Path(tmp.name)
+        os.replace(tmp_path, path)
         return True
     except Exception:
+        if tmp_path and tmp_path.exists():
+            try:
+                tmp_path.unlink()
+            except OSError:
+                pass
         return False
🧰 Tools
🪛 Ruff (0.15.2)

[warning] 99-99: Consider moving this statement to an else block

(TRY300)


[warning] 100-100: Do not catch blind exception: Exception

(BLE001)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tui/hookwise_tui/data.py` around lines 92 - 101, write_config currently
writes directly to the target file and may produce truncated configs on
interruption; change it to perform an atomic write by writing the YAML to a
temporary file in the same directory (use the same parent from
_effective_config_path(path) / Path.parent), flush and fsync the temp file,
close it, then atomically replace the target with os.replace (or Path.rename) so
the switch is atomic; preserve the existing return True/False behavior and catch
exceptions around the write/replace steps, cleaning up the temp file on failure.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferring. write_config is called infrequently (on user toggle in the TUI) and writes small YAML files. The window for truncation from interruption is negligible. Atomic writes add complexity (temp file management, cleanup on failure, cross-platform rename semantics) that isn't justified for this use case. Will revisit if config corruption is ever reported.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vishnujayvel, understood — that's a reasonable trade-off for the current usage pattern. Acknowledged.

Comment on lines +322 to +326
config = read_config()
if "status_line" not in config or not isinstance(config.get("status_line"), dict):
config["status_line"] = {}
config["status_line"]["segments"] = active

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Persisted segment toggles are not wired to the CLI renderer path.

This writes status_line.segments, but src/cli/commands/status-line.ts still renders with renderTwoTier(DEFAULT_TWO_TIER_CONFIG, cache). So saved toggles won’t affect actual status-line output.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@tui/hookwise_tui/tabs/status.py` around lines 322 - 326, The saved toggles
are written to config["status_line"]["segments"] but the CLI renderer still uses
renderTwoTier(DEFAULT_TWO_TIER_CONFIG, cache), so persisted toggles have no
effect; update the CLI renderer (src/cli/commands/status-line.ts) to load the
persisted config (status_line.segments) and merge or replace
DEFAULT_TWO_TIER_CONFIG with those segments before calling renderTwoTier (or
alternatively read the same cache key you persist to), ensuring renderTwoTier
receives the persisted segments instead of always using DEFAULT_TWO_TIER_CONFIG.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Known gap — the TUI persists toggle state to config.status_line.segments, but the CLI always renders from DEFAULT_TWO_TIER_CONFIG. This is intentional for this PR: the TUI config persistence was added for TUI preview functionality, and the CLI → config bridge is a separate feature. Tracking for a future iteration.

- Remove `set -e` from agent-tracker.sh to honor fail-open contract
- Make middleSegments/showSeparator optional in TwoTierConfig interface

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant